ideally we'd have these checks be part of CreateV2ViewExec but by the time we reach CreateV2ViewExec the underlying `query' doesn't contain the info needed to detect these cases here.

If the identifier is a ResolvedIdentifier, don't we already know that a temporary view does not exist? That's checked when resolving views. Does the UnresolvedIdentifier has allowTemp set to false?

what the check does is to make sure that a view doesn't reference a temporary/global view. We particularly test for these 2 cases in createViewReferencingGlobalTempView() / createViewReferencingTempView()

Do we want the parsed logical plan here? Is the purpose to run the analyzer rules to make sure that it is valid?

My guess is that this came from the Spark implementation, which probably uses the logical plan here for when there is no SQL text (which would be why originalText is optional). Unless I'm missing something, I think what we should do is not have the parsed query. Instead, I would have a validation that parses and analyzes the query and throws an exception if analysis fails (because of bad references, missing tables, or reliance on temporary views).

Does this matter if the column aliases were specified?

I think we want to perform this check, because Spark does the same and fails if you provide duplicate columns in the underlying query:

spark-sql (default)> create temporary view tempv2 as select id, id from iceberg1.foo WHERE id < 12;
[COLUMN_ALREADY_EXISTS] The column `id` already exists. Consider to choose another name or rename the existing column.

I've left that check and also added a test for this

If there are aliases, we could succeed though.

After looking into this a bit more, query does correspond to originalText. That's good because we don't have to call the parser separately and this ensures that the query can be analyzed correctly. We should keep this design for the logical plan.

I think there are a few things that we can improve, though. Since the query plan is linked in like this, it will be automatically analyzed so this should already catch things like trying to project a missing column (by the way, we should have a test for that). It will also catch missing relations (tables and other views) but we probably need to handle resolution a bit differently --- we want view resolution to happen just like it would when loading a view. I think that means that we should call our own code from createViewRelation to rewrite identifiers. (Ideally, we could alias here as well, but it may need some special handling if GetColumnByOrdinal doesn't work with an undetermined type.)

Rewriting identifiers needs to happen immediately so that Spark doesn't substitute any temporary views. I think this needs to be done before ResolveViews so we should do it in RewriteViewCommands.

To summarize:

• RewriteViewCommands should call our code to rewrite identifiers in the parsed query (and alias columns, if possible)
• If RewriteViewCommands can't apply the aliases, then we will need a rule that applies them once the query is resolved
• RewriteViewCommands should also check for temporary view references and fail if any are found (and maybe temporary functions as well?)
• Checking for the right number of columns should be done in the checker (validation phase) if it isn't done already because we use GetColumnByOrdinal for aliases
• We should drop the query when possible because it shouldn't be needed after the analysis phase. Instead we should get the schema of the query (after aliasing) and pass that along.

@nastra, this can't be done here. It needs to be done before any of Spark's rules run. Otherwise, a temporary view may already have been substituted into the plan and could allow the checks to pass even though the view is invalid.

This rule is only responsible for applying the column aliases and comments. Rewriting the identifiers should be done in RewriteViewCommands.

I think the query needs to have CTE substitution run before running the temporary object check. Otherwise, there could be conflicts between CTE substitution and temporary names. If there is a conflict, the CTE should be used.

you're absolutely right. I've done that and also added a test

I think I'd also be fine with not tracking this and passing an empty array through. That's not correct for Spark, but it would work for us since Iceberg doesn't store these.

technically we don't need to track it in this PR but we'll eventually need it when we show the properties of views and such. I'd probably keep it here but let me know if you'd like me to remove it here and introduce it in an upcoming PR

It seems reasonable to include it. Let's just leave it as-is since you even have a test for it.

I think we want a test for a CTE that conflicts with a temporary table name.

not sure if you meant a temp view name here (instead of temp table name) but I added a test where a CTE and a temp view both have the same name and when data is being read, the CTE takes precedences

That's correct. I did mean temporary view name. Thanks!

Looks good.